Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use roles from the embedded-cluster config #4133

Merged

Conversation

laverya
Copy link
Member

@laverya laverya commented Nov 14, 2023

What this PR does / why we need it:

Includes the ability to specify custom node roles that include node labels when joining a node to the cluster.

I think no release notes as this is still in a pre-beta state

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Steps to reproduce

Does this PR introduce a user-facing change?

Upgrade to Go 1.21

Does this PR require documentation?

Comment on lines +3 to +5
go 1.21.0

toolchain go1.21.3
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to be required as part of importing embedded-cluster-operator

Copy link
Contributor

@cbodonnell cbodonnell Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make it an easier import if there was a separate module for the kinds/apis as we have for kotskinds and kurlkinds? probably not necessary now, but thinking ahead for when the project inevitably grows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be something to consider certainly

@laverya laverya marked this pull request as ready for review November 15, 2023 03:29
if err != nil {
return nil, fmt.Errorf("failed to list installations: %w", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there scenarios where the list returned could be empty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an installation created with every embedded-cluster install, so ideally no

cbodonnell
cbodonnell previously approved these changes Nov 15, 2023
@laverya laverya merged commit 36dff42 into main Nov 16, 2023
161 of 162 checks passed
@laverya laverya deleted the laverya/sc-92433/in-the-admin-console-show-the-name-for-the branch November 16, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants